-
-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
allow_to_return_raw_or_keyword_list #32
allow_to_return_raw_or_keyword_list #32
Conversation
Hi @alexdesousa . Are you planning to still maintain this library? |
@dkuku I'm still planning on maintaining the project, but lately it's been hard to find the time. I was checking your PR yesterday and I'm planning on adding some review comments. Mainly some stuff I find are redundant. However, I wanted to thank you for your interest in this humble project 😀 |
That's cool, I have some more work in progress :) |
src/ayesql_parser.yrl
Outdated
@@ -1,33 +1,48 @@ | |||
Nonterminals queries named_queries named_query fragments. | |||
Terminals '$name' '$docs' '$fragment' '$named_param'. | |||
Terminals '$name' '$type' '$docs' '$fragment' '$named_param'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkuku I'm sorry, but I don't fully understand the purpose of this type
in the AyeSQL language. Can you please elaborate? I understand a possible need to parameterize scripts, but I don't think modifying the parser is a good approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this was an accidental commit. It should not be here. But to explain it - some of the libraries accept a modifier for the query:
-- name: my_query^
means select one similar to Repo.one
-- name: seed#
allows to run multiple queries from a file - I'm using this for seeding database for tests in one of my projects. I just don't like the modifier and think that having additional comment makes more sense. Especially that function names with !
have special meaning in elixir.
https://nackjicholson.github.io/aiosql/defining-sql-queries.html
https://github.com/krisajenkins/yesql#insert-returning-autogenerated-keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, for this type of seed file, it's better to leverage other stuff like EEx
templates instead of AyeSQL parser. Mainly, because you can still parameterize them using the assigns
(see more here) and you don't really need functions at run time for the seeds, so they could be simply uncompiled elixir scripts (similar to the scripts used in Ecto
) e.g. so if we have the following file:
-- ./priv/my_seeds.sql.eex
INSERT INTO users (email, password)
VALUES ('<%= @email %>', '<%= @password %>');
And then load the file in an elixir script like this:
# ./my_seeds.exs
# ... omitting Ecto.Repo config ...
assigns = [
username: "[email protected]",
password: "whatever" |> :crypto.hash(:sha256) |> Base.encode16()
]
seeds =
"./priv/my_seeds.exs"
|> File.read!()
|> then(&EEx.eval_string(&1, assigns: assigns))
# The run a transaction with the raw SQL statement in the variable `seeds`.
Finally, you could run everything with mix run
or elixir
commands.
lib/ayesql/compiler.ex
Outdated
@@ -181,7 +189,47 @@ defmodule AyeSQL.Compiler do | |||
end | |||
|
|||
@spec create_query(query()) :: Macro.t() | |||
defp create_query({name, docs, fragments}) do | |||
defp create_query({name, :script, docs, fragments}) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkuku There are several reasons why I don't think modifying the parser is a good approach (see my following comments):
lib/ayesql/compiler.ex
Outdated
AyeSQL.Core.evaluate(content, params, context) do | ||
if run? do | ||
statement | ||
|> String.split(";", trim: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkuku There's parsing outside of the parser for a specific case. This should be handled somehow inside the parser in case it needs to be supported. This raises new questions though:
- Should we split the parser between
normal
andscript
types? - Show
script
types accept more than onescript
(so more than one generated function) from the same file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we wan't to implement this feature it needs some discussion around it. And be wrapped in transaction. The blocker I had was that transactions are differently named in postgres comparing to mysql so this would need to be somehow configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, you can rely on Ecto.Repo.transaction/2
. Those are general enough that do not care about the underlying database.
lib/ayesql/compiler.ex
Outdated
statement | ||
|> String.split(";", trim: true) | ||
|> Enum.map(&String.trim(&1)) | ||
|> Enum.each(&__MODULE__.run!(%{query | statement: &1}, options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkuku Splitting a statement with ;
and then running each statement by itself makes the script operation unsafe as it doesn't happen inside a transaction.
Instead of this change, wouldn't it be better to use an actual script and use Postgrex or Ecto's APIs to do the sustitution?
7b28bf5
to
02b671c
Compare
@dkuku I finally got some time to push the CI fixes. You'll need to rebase your branch. |
6b5a013
to
59069f4
Compare
@alexdesousa I updated the branch |
@dkuku After those changes are committed, we can merge the PR into master. |
This allows to get the raw data data returned by postgrex for use with for example table_rex.
It also allows to convert to keyword list which is sorted comparing to maps.
Default
into
param can be passed to use function.